Had a look, it works on my system and I can now do things like isinstance(k, Kambites) and subclass on Kambites and use Kambites as a type hint, which is great!

However, member functions that return a reference to the cxx object now cause unintuitive behavior, e.g.

k1 = Kambites(Word=str)
assert isinstance(k1, Kambites) # Passes
k2 = k1.init()
assert isinstance(k2, Kambites) # Fails

since the init function returns an instance of the underlying KambitesString or KambitesWord class not the Kambites wrapper class. In the case of Kambites one fix would just be to manually wrap these functions with an extra Kambites conversion at the end. Alternatively we could just write some function converting the internal instances to the appropriate wrapped instances and modify the cxxwrapper to automatically wrap them.

E.g. something like

_internal_to_wrapper_dict = {
    KambitesWords: Kambites,
    KambitesStrings: Kambites,
    PresentationWords: Presentation,
    PresentationStrings: Presentation, # Every time we add a wrapper register it here
}

def internal_to_wrapper(x):
    if type(x) in _internal_to_wrapper_dict:
        return _internal_to_wrapper_dict[type(x)](x)
    return x

and then we wrap the return of __getattr__ in CxxWrapper with internal_to_wrapper. Though I guess we might need to do this recursively if the return type is a list or tuple. This does feel like a hacky solution though.

Another way would be to somehow make KambitesWords and KambitesStrings be recognized as subclasses of Kambites. But I have no idea how to do this or if it might break everything if we try. I wonder if pybind11 has some functionality built in for declaring something like a base class for templated classes. So we somehow get pybind11 to output a KambitesBase class, which has no functions defined on it, but is a parent class for both KambitesWord and KambitesString.

Thanks for the comments @reiniscirpons, that's an oversight on my behalf about the return value of init et al.

Some comments: I don't think the new approach really uses __getattr__ in CxxWrapper at all, for a couple of reasons (mostly pointed out by @Joseph-Edwards):

1. if we don't define a method with name foo in, say, Kambites, then tab completion doesn't work in ipython. This is arguably not super important, but it's nice if it does work;
2. no documentation is generated for foo. This is critical, for example, in Congruence and Kambites some methods are declared explicitly in the class definition (has and get and number_of_classes in Congruence), and others are implicit and "installed" via _copy_cxx_mem_fns. If we don't do _copy_cxx_mem_fns, then there'd be no doc for the methods installed this way in Congruence. On the other hand, if we don't _copy_cxx_mem_fns, then we could get the doc from CongruenceWords in _libsemigroups_pybind11 but then we won't have the doc for the explicit methods such as get and has.

All of that is to say that I think the best thing is just to explicitly declare the methods that should return self, and add a decorator to them, as we've done with number_of_classes for example. Sound ok?

I don't think the new approach really uses __getattr__ in CxxWrapper at all

Ah ok, I overlooked the copy_cxx_mem_fns function and misunderstood how the wrapping works.

All of that is to say that I think the best thing is just to explicitly declare the methods that should return self, and add a decorator to them, as we've done with number_of_classes for example. Sound ok?

Sounds good, this is probably the simplest fix. One extra issue might be if we have any member function which returns a different wrapped type (e.g. a Congruence returning a ToddCoxeter), but for Congruence this is already handled a with the get function and I imagine all other such conversion go through the to function, so should be fine.

I think I've accounted for the issues that you pointed out @reiniscirpons if you have any other comments, please let me know! O/w I'll continue with the other classes

Looks good to me. I will implement a similar approach for Stephen in the meeting today.

In 6019bc3, we update our custom extension to look for inserted signatures at the start of a docstring. If the name of the function in the inserted signature does not match the name of the function being documented (e.g. knuth_bendix_non_trivial_classes vs non_trivial_classes) then we remove those inserted lines from the doc.

This means that we can remove the options.disable_function_signatures() stuff from FroidurePin if that is desired.

I think this is now complete from my perspective @Joseph-Edwards

I think this is now complete from my perspective @Joseph-Edwards

Sweet! Thanks @james-d-mitchell. I'll try and review the changes/use the package over the next couple of days and try and get it merged.

make check is currently failing for me, because we can't construct Stephen objects from Presentation objects.

Oooh right I merge that but didn't check, I'll fix that now

I think it would be very helpful to have a way of formatting the rst code, but some of the changes introduced in 2fc2c51 don't interact with Sphinx's autodoc stuff very well.

For example, in paths.rst, the entries in the autosummary are indented by 10 spaces, some files use five spaces, and most use 4.

It looks like most of the stuff has been formatted well though.

I think it would be very helpful to have a way of formatting the rst code, but some of the changes introduced in 2fc2c51 don't interact with Sphinx's autodoc stuff very well.

For example, in paths.rst, the entries in the autosummary are indented by 10 spaces, some files use five spaces, and most use 4.

It looks like most of the stuff has been formatted well though.

Hmmm I’m not seeing any warnings when I run make doc, are you? If so, can you please post them? Thanks!

Hmmm I’m not seeing any warnings when I run make doc, are you? If so, can you please post them? Thanks!

I don't get any warnings either. It just seemed odd that the .rst files now seem to have inconsistent amounts of indentation.

As far as I can tell, this hasn't changed the appearance of the doc itself.

Hmmm I’m not seeing any warnings when I run make doc, are you? If so, can you please post them? Thanks!

I don't get any warnings either. It just seemed odd that the .rst files now seem to have inconsistent amounts of indentation.

As far as I can tell, this hasn't changed the appearance of the doc itself.

Ah right, I think it was pretty inconsistent before too, so I'm not sure it's now worse than before. I should file an issue with docstrfmt, also there were some changes that it made that did break the doc, which I fixed.

Are you happy for this to be merged?

Ah right, I think it was pretty inconsistent before too, so I'm not sure it's now worse than before. I should file an issue with docstrfmt, also there were some changes that it made that did break the doc, which I fixed.

Are you happy for this to be merged?

Yeah in that case I'm happy to merge. Thanks again @james-d-mitchell